Reformat test structure and imports #50

TylerYep · 2020-09-27T22:02:35Z

Hi, I was interested in using this project, but ran into some issues with pip installing the package. I ended up renovating the project to fit my needs, and thought there was a chance you might be interested in using the changes.

Please feel free to take these changes or throw them away, I'm mainly just leaving this PR here as a suggestion of possible improvements!

Changes included:

Run isort .
Run black .
Rename didyoumean files to make imports easier
Add a separate tests/ folder to avoid mixing source files with test files
Resolve several deprecation warnings that are raised when running pytest

SylvainDe · 2020-09-28T04:39:20Z

Thank you so much for your pull request!
I remember that I struggled trying to make this installable with pip for a very limited success ( #3 ).

I'll have a look at the details and integrate it as soon as possible.

SylvainDe

Thank you so much for the hard work you've put inthis contribution!

I have not gone through the whole review yet.

My first concern would be to ensure that the CI runs smoothly on all Python versions.
Currently, the pre-checks are failing because of the line length - the best option is to change the pycodestyle limit in the travis configuration.
Also, I suspect other tests would fail at runtime (at least on the older Python versions)

If you decide to spend more time on this, I would appreciate if things could be split into pull-requests with a somehow clearer scope. For instance, one for black (and typo), one for the reorganisation, etc. If you can't be bothered, that's fine as well.

Let me know if there's anything I may help you with

All the best!

SylvainDe · 2020-09-28T04:41:50Z

didyoumean/api.py

@@ -110,6 +113,3 @@ def didyoumean_disablehook():
    """Function to set hooks to their normal value."""
    sys.excepthook = sys.__excepthook__
    set_ipython_custom_exc(None)
-
-# NOTE: It could be funny to have a magic command in Python


Do you know why this got removed ?

SylvainDe · 2020-09-28T04:47:00Z

tests/dre_test.py

-            self.assertRegexpMatches(name, regex)
-        for name in ['1a']:
-            self.assertNotRegexpMatches(name, regex)
+    # def test_var_name(self):


Sorry for that one - I guess it caused you troubles.
Would you remember which name led to problems by any chance ?

SylvainDe · 2020-09-28T04:54:23Z

tests/dre_test.py

@@ -31,7 +32,7 @@ def re_matches(self, text, regexp, results):
        and groupdict().
        """
        groups, named_groups = results
-        self.assertRegexpMatches(text, regexp)   # does pretty printing
+        self.assertRegex(text, regexp)  # does pretty printing


The reasoning for this was to ensure the lib was usable and testable under all Python version >= 2.6.
Indeed, somes of the reasons caught and handled are somehow related to running a piece of Python code with the wrong version of the interpreter (not as common now but a few years back, using 2.7 instead of 3.0+ or the other way round was a common gotcha).
Would it be possible to revert that part of the patch ?

SylvainDe · 2020-09-28T04:58:29Z

didyoumean/dre.py

+    r"^(?:local|free) variable '(?P<name>{0})' "
+    r"referenced before assignment(?: in enclosing scope)?$".format(VAR_NAME)
+)
+NAMENOTDEFINED_RE = r"^(?:global )?name '(?P<name>{0})' " r"is not defined$".format(


Now, this line (and a few others) are considered too long by the call to pycodestyle on travis.
Would it be possible to either:

tweak the travis config to disable the corresponding warning ( E501 )

run black with a different line length limit
(I think option 1 is both an easier and cleaner option)

https://travis-ci.org/github/SylvainDe/DidYouMean-Python/jobs/730786126

didyoumean/internal.py

SylvainDe · 2020-09-28T05:22:57Z

Note: I don't know why this does not appear more explicitly here but you can find the Travis runs for your Pull Request here: https://travis-ci.org/github/SylvainDe/DidYouMean-Python/builds/730786103 .

TylerYep · 2020-09-28T05:28:08Z

Are you dedicated to supporting Python 2.6 at this point? All versions of up to Python 3.5 have reached end-of-life by now (https://endoflife.date/python), so it seems wise to move along with this trend - especially if this project is not currently used by many people yet.

It would be pretty easy to split this into multiple changes - I can do it sometime this week. Thanks for reviewing the changes!

SylvainDe · 2020-09-29T08:54:21Z

This looks great! Thanks for the additional work you've gone through.

Unfortunately, I can see that the CI is failing - https://travis-ci.org/github/SylvainDe/DidYouMean-Python/pull_requests . Would you have any idea about what happened ?

As for the support for older Python version, I'd rather keep it until it does bring a lot more troubles while moving forward. If it is only for DeprecationWarning, I'd rather add a few wrappers around the unit-test methods to ensure that everything works smoothly in all cases.

Correct spelling for "dictionnary" is "dictionary". This is one of the things hilighted in: #50

SylvainDe · 2020-10-04T09:02:41Z

Hello @TylerYep , I've performed a few changes which will unfortunately lead to merge issues.
On the positive side: the DeprecationWarnings are now fixed and I've removed a few pep8 errors so that Black can be used safely.

To you mind resubmitting your code organisation PR ? (If you want, I can perform it myself based on the work you've done and try to give you the proper attribution). Similarly, I can perform the call to Black in a different commit if you want.

Please let me know how you want to handle these and thanks for your work!

TylerYep · 2020-10-04T18:13:48Z

Resubmitted the PR. I can go ahead and create the PR for the black formatting, but could you fix the travis CI integration first?
I believe you just need to:

Add the Travis-CI app in the Github Marketplace
Add the DidYouMean project to TravisCI (I believe this is already done)
Remove the scrutinizer app from this project

As a side note, I would also really recommend upgrading to a newer toolchain for linting + testing. I believe the more common standard now is pytest + flake8 instead of unittest + pep8/pycodestyle/pyflakes.

Pytest should be able to execute all unittests without any modifications, but with a better test-discovery. The following commands should do everything correctly and concisely in the .travis.yml:

isort .
black .
flake8 .
pytest
codecov

SylvainDe · 2020-10-08T07:24:01Z

Thanks for all the suggestions ! It looks like I have not followed the latest trends on a few aspects indeed.
I've disabled scrutinizer for all PRs.
As for Travis, a few things changed (apps, migration from org to com) and I did not quite manage to make it work yet. I'll try again later on.

SylvainDe requested changes Sep 28, 2020

View reviewed changes

TylerYep force-pushed the master branch 5 times, most recently from 24ebcef to 7aba306 Compare September 28, 2020 19:51

TylerYep changed the title ~~Run Black + isort, reformat test structure~~ Reformat test structure and imports Sep 28, 2020

SylvainDe added a commit that referenced this pull request Oct 4, 2020

Fix typo (hilighted by TylerYep)

418b9c2

Correct spelling for "dictionnary" is "dictionary". This is one of the things hilighted in: #50

separate test files and rename imports

fe7dd1e

TylerYep force-pushed the master branch from 7aba306 to fe7dd1e Compare October 4, 2020 18:01

alexmojaki mentioned this pull request Oct 31, 2020

Can't use from pip install #52

Open

TylerYep closed this Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reformat test structure and imports #50

Reformat test structure and imports #50

TylerYep commented Sep 27, 2020 •

edited

Loading

SylvainDe commented Sep 28, 2020

SylvainDe left a comment

SylvainDe Sep 28, 2020

SylvainDe Sep 28, 2020

SylvainDe Sep 28, 2020

SylvainDe Sep 28, 2020

SylvainDe commented Sep 28, 2020

TylerYep commented Sep 28, 2020

SylvainDe commented Sep 29, 2020

SylvainDe commented Oct 4, 2020

TylerYep commented Oct 4, 2020 •

edited

Loading

SylvainDe commented Oct 8, 2020

Reformat test structure and imports #50

Reformat test structure and imports #50

Conversation

TylerYep commented Sep 27, 2020 • edited Loading

SylvainDe commented Sep 28, 2020

SylvainDe left a comment

Choose a reason for hiding this comment

SylvainDe Sep 28, 2020

Choose a reason for hiding this comment

SylvainDe Sep 28, 2020

Choose a reason for hiding this comment

SylvainDe Sep 28, 2020

Choose a reason for hiding this comment

SylvainDe Sep 28, 2020

Choose a reason for hiding this comment

SylvainDe commented Sep 28, 2020

TylerYep commented Sep 28, 2020

SylvainDe commented Sep 29, 2020

SylvainDe commented Oct 4, 2020

TylerYep commented Oct 4, 2020 • edited Loading

SylvainDe commented Oct 8, 2020

TylerYep commented Sep 27, 2020 •

edited

Loading

TylerYep commented Oct 4, 2020 •

edited

Loading